fix(contact): safe spam handling and actionable validation - #168
Conversation
Cover the static citation path with exact List.of overload evidence from Java 21 and 24, preserving strict anchor fidelity.
Treat non-positive client render times as spam before reserving rate-limit capacity or attempting SMTP delivery, while preserving the 202 response.
Give each visible contact field user-facing Zod messages so required and malformed input no longer collapses to generic dependency copy. Test exact inline guidance and preserve client-side rejection.
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe changes add explicit contact-form validation messages, reject non-positive render timestamps as spam, and add regression coverage for multi-version citation discovery and contact submission handling. ChangesContact validation feedback
Timestamp spam detection
Citation discovery regression
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
coderabbitai
Bot
added
bug
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
frontend/src/lib/components/ContactPage.test.ts (1)
91-93: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the canonical validation-message owner.
Lines [91]-[93] duplicate messages owned by
frontend/src/lib/validation/schemas.ts. Export the canonical message constants and import them here. Keep thegetByTextassertions so the test still verifies the rendered guidance without creating a second source of truth.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/lib/components/ContactPage.test.ts` around lines 91 - 93, Export the canonical validation-message constants from the schemas module and import them into the ContactPage test. Update the three getByText assertions to use those constants instead of duplicating literal messages, preserving the existing rendered-guidance checks.Source: Coding guidelines
src/test/java/com/williamcallahan/javachat/web/ContactControllerTest.java (1)
138-155: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCover the rate-limit boundary in this regression test.
This test proves that
JavaMailSender.sendis not called. It does not prove that a non-positive timestamp leavesacceptedSubmissionsPerIpunchanged. If the spam check moved after the counter increment, these three invocations could still return202and the test would pass.Add a same-IP sequence with four non-positive submissions followed by one legitimate submission. Assert
202and one mail send for the legitimate submission. This covers the stated PR requirement that non-positive timestamps are handled before rate limiting.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/test/java/com/williamcallahan/javachat/web/ContactControllerTest.java` around lines 138 - 155, Extend nonPositiveRenderTimestampIsDroppedSilentlyWithoutSendingMail to submit four non-positive timestamp requests from the same IP, then submit one legitimate request from that IP. Assert each request remains accepted, verify the legitimate request triggers exactly one JavaMailSender.send call, and retain the existing no-mail assertion for the invalid submissions to confirm they do not consume the rate-limit counter.src/test/java/com/williamcallahan/javachat/service/RetrievalServiceTest.java (2)
468-491: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winVerify the multi-release dispatch explicitly.
The exact
when(...)stub does not assert call count or reject an extra retrieval or reranking call. Addverify(...)forsearchDocumentationCitationsOutcomes(...)with both release constraints. Also verify thatsearchOutcome(...)andrerank(...)are never called. This protects the route contract, not only the returned citations.Suggested assertion
assertEquals(0, citationOutcome.failedConversionCount()); + verify(hybridSearchService) + .searchDocumentationCitationsOutcomes( + eq(exactComparisonQuery), + eq(10), + eq(List.of(java21Constraint, java24Constraint)), + anyLong()); + verify(hybridSearchService, never()) + .searchOutcome(anyString(), anyInt(), any(RetrievalConstraint.class), anyLong()); + verify(rerankerService, never()).rerank(anyString(), anyList(), anyInt(), anyLong());🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/test/java/com/williamcallahan/javachat/service/RetrievalServiceTest.java` around lines 468 - 491, Add explicit Mockito verification after discoverCitations in the multi-release test: verify searchDocumentationCitationsOutcomes is called exactly once with exactComparisonQuery, 10, both release constraints, and anyLong; verify searchOutcome(...) and rerank(...) are never called. Use the existing service and constraint symbols in the test.
463-467: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the new magic literals into named constants.
The test introduces inline release identifiers,
10, query text, fixture IDs, hashes, theof(E,E)anchor, and expected URLs. Define domain-qualified constants and use them in the constraints, fixtures, stub, and assertions. Keep expected URL constants independent fromDocsSourceRegistryso the test remains an exact-output oracle.As per coding guidelines,
No inline numbers (except 0, 1, -1) or strings; define named constants (Magic Literals).Also applies to: 477-487
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/test/java/com/williamcallahan/javachat/service/RetrievalServiceTest.java` around lines 463 - 467, Extract the inline release identifiers, query text, fixture IDs, hashes, the of(E,E) anchor, and expected URLs used by the RetrievalServiceTest scenario into clearly named, domain-qualified constants. Reuse those constants in officialDocumentationConstraint, the exact overload fixtures, stubs, and assertions, including the additional values in the referenced lines. Keep expected URL constants defined independently from DocsSourceRegistry so assertions remain exact-output oracles.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@frontend/src/lib/validation/schemas.ts`:
- Around line 137-147: Define domain-qualified named constants for the contact
name, email, and message validation errors in ContactSubmissionSchema’s module,
then use those constants instead of inline literals. In
frontend/src/lib/components/ContactPage.test.ts lines 91-93, import and use the
canonical constants rather than repeating the messages; update both affected
sites accordingly.
---
Nitpick comments:
In `@frontend/src/lib/components/ContactPage.test.ts`:
- Around line 91-93: Export the canonical validation-message constants from the
schemas module and import them into the ContactPage test. Update the three
getByText assertions to use those constants instead of duplicating literal
messages, preserving the existing rendered-guidance checks.
In
`@src/test/java/com/williamcallahan/javachat/service/RetrievalServiceTest.java`:
- Around line 468-491: Add explicit Mockito verification after discoverCitations
in the multi-release test: verify searchDocumentationCitationsOutcomes is called
exactly once with exactComparisonQuery, 10, both release constraints, and
anyLong; verify searchOutcome(...) and rerank(...) are never called. Use the
existing service and constraint symbols in the test.
- Around line 463-467: Extract the inline release identifiers, query text,
fixture IDs, hashes, the of(E,E) anchor, and expected URLs used by the
RetrievalServiceTest scenario into clearly named, domain-qualified constants.
Reuse those constants in officialDocumentationConstraint, the exact overload
fixtures, stubs, and assertions, including the additional values in the
referenced lines. Keep expected URL constants defined independently from
DocsSourceRegistry so assertions remain exact-output oracles.
In `@src/test/java/com/williamcallahan/javachat/web/ContactControllerTest.java`:
- Around line 138-155: Extend
nonPositiveRenderTimestampIsDroppedSilentlyWithoutSendingMail to submit four
non-positive timestamp requests from the same IP, then submit one legitimate
request from that IP. Assert each request remains accepted, verify the
legitimate request triggers exactly one JavaMailSender.send call, and retain the
existing no-mail assertion for the invalid submissions to confirm they do not
consume the rate-limit counter.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 65aa880c-1ac0-4d9e-8e83-6062ca53e06e
📒 Files selected for processing (5)
frontend/src/lib/components/ContactPage.test.tsfrontend/src/lib/validation/schemas.tssrc/main/java/com/williamcallahan/javachat/application/contact/ContactSubmissionUseCase.javasrc/test/java/com/williamcallahan/javachat/service/RetrievalServiceTest.javasrc/test/java/com/williamcallahan/javachat/web/ContactControllerTest.java
There was a problem hiding this comment.
Pull request overview
This PR strengthens contact-form spam defenses by treating missing/zero/negative renderedAt timestamps as spam (while preserving the existing HTTP 202 response), improves client-side validation messaging for contact submissions, and adds regression tests around contact admission plus multi-version Java API citation discovery.
Changes:
- Backend: treat
renderedAt == nullorrenderedAt <= 0as spam and drop the submission before rate limiting / SMTP. - Frontend: provide actionable Zod validation messages for name/email/message fields; add UI test expectations for those messages.
- Tests: add coverage ensuring non-positive
renderedAtsubmissions never send mail; add adiscoverCitationsmulti-version citation test.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/test/java/com/williamcallahan/javachat/web/ContactControllerTest.java | Adds parameterized coverage ensuring non-positive renderedAt values are silently accepted (202) without sending mail. |
| src/test/java/com/williamcallahan/javachat/service/RetrievalServiceTest.java | Adds a discoverCitations multi-version Java 21/24 List.of(E,E) citation regression test (but does not reproduce Issue #165’s failure mode). |
| src/main/java/com/williamcallahan/javachat/application/contact/ContactSubmissionUseCase.java | Updates spam detection to treat missing or non-positive renderedAt as spam. |
| frontend/src/lib/validation/schemas.ts | Improves contact schema validation messages to be user-actionable. |
| frontend/src/lib/components/ContactPage.test.ts | Asserts the new validation copy is rendered and that invalid submissions skip the API call. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Keep field-specific error copy with the contact schema while preserving independent rendered-copy expectations at the UI test boundary.
Prove spam timestamps leave same-IP quota untouched and lock exact multi-release citation discovery to its dedicated dispatch path.
Summary
Contact submissions now drop non-positive render timestamps before rate-limit or SMTP work and show actionable client-side validation guidance. Exact Java 21 and Java 24
List.of(E,E)citation coverage confirms the reported citation failure is not reproducible when both releases provide authoritative exact-member evidence.Changes by Category
Bug Fixes
renderedAtvalues as spam while preserving the indistinguishable202 acceptedresponse.Refactoring
Testing
0,-1, andLong.MIN_VALUEtimestamps do not send mail or consume the same-IP allowance before a legitimate request.Test plan
31043583245: frontend, build, and Docker smoke passed.31043582929: frontend, build, and Docker smoke passed.e9g7gyluymfkocvt97vuo5bwfinished at exact SHAb368011c2785bd5d85bada4f9552c90ff6393625; readiness and liveness areUP./api/contactrequest.List.of(E,E)chat dogfood returned both Oracle sources with matching anchors.Breaking changes
None.
Related issues